[voq] VOQ cli show commands implementation #1689

vganesan-nokia · 2021-06-22T19:49:36Z

What I did

Implementation of the following show commands for VOQ chassis.

show chassis system-ports [<system port>] {-n <namespace>}, if multi-asic voq chassis
show chassis system-ports [<system port>] , if single asic voq chassis
show chassis system-neighbors [<nbr ip address>] [-n <asic name>]
show chassis system-lags [<system lag>] [-n <asic name>] [-l <linecard or host name>]

The existing chassis commands under "show chassis_modules" and
"config chassis_modules" have been renamed (moved) under
"show chassis modules" and "config chassis modules" respectively

How I did it

Modified sonic utilities scripts

How to verify it

In VOQ chassis have system ports and port channel configurations. Have some neighbors attached to some front panel ports.
Run the CLI commands mentioned above form line card. The commands will show the outputs as mentioned in the design document update PRs [VOQHLD] Update for cli commands SONiC#801, [VOQ LAG HLD] Updates for show commands SONiC#802 and Update for changing chassis_modules command SONiC#805

Previous command output (if the output of a command-line utility has changed)

Commands changes:

The commands under show chassis_modules have been moved/re-named to show chassis modules
The commands under config chassis_modules have been moved/re-named to config chassis modules

New command output (if the output of a command-line utility has changed)

No change in the output of the existing commands.

lgtm-com · 2021-06-22T19:58:17Z

This pull request introduces 3 alerts when merging 1dcbec93ed30d0030feb0ed35cee9055bbb74b2d into 5708497 - view on LGTM.com

new alerts:

3 for Unused local variable

anshuv-mfst · 2021-06-30T16:15:28Z

Hi @ysmanman - could you please review , thanks

arlakshm

as comments

arlakshm · 2021-07-07T07:18:54Z

tests/chassis_modules_test.py

+            System Port Name    Port Id    Switch Id    Core    Core Port    Speed
+----------------------------  ---------  -----------  ------  -----------  -------
+   Linecard1|Asic0|Ethernet0          1            0       0            1     100G
+   Linecard1|Asic0|Ethernet0          1            0       0            1     100G


The same output lines are repeated, is this because the same system-port information is present in every asic ?
This output is be confusing.
Can we either add

a new column asic_name to indicate which asic this info is retrived or

make the -n <asicname> mandatory for the multi asic linecards , so that the we don't display same system port information multiple times

Yes. The same information will be repeated multiple times - as many times as the number of asics. This is because we have identical system port config in all asics. We display this multiple times so that one can verify for the requirement of indentical system port configuration in all asics.

The option to make the mandatory seems better than displaying all entries with new column since there will be lots of entries if there are many asics.

I'll implement changes to make the asic name mandatory in the command. Thanks for the suggestions.

Fixed. Namespace option is made "required" option.

arlakshm · 2021-07-07T07:24:24Z

scripts/voqutil

+    """
+    Get CHASSIS_APP_DB SYSTEM_NEIGH table Keys
+    """
+    system_neigh_keys = chassis_app_db.keys(chassis_app_db.CHASSIS_APP_DB, "SYSTEM_NEIGH|*")


Use SYSTEM_NEIGH_TABLE_PREFIX iso SYSTEM_NEIGH| here ?

Fixed, here and in other places also.

arlakshm · 2021-07-07T07:29:08Z

scripts/voqutil

+        self.db = None
+        self.config_db = None
+        self.table = []
+        self.multi_asic = multi_asic_util.MultiAsic(


Looks self.multi_asic like this is not used ?

Fixed. Removed since this is not used

arlakshm · 2021-07-07T07:29:43Z

scripts/voqutil

+        self.config_db = None
+        self.system_lag_name = system_lag_name
+        self.table = []
+        self.multi_asic = multi_asic_util.MultiAsic(


Same as above, looks self.multi_asic like this is not used ?

Fixed. Removed since this is not used.

lgtm-com · 2021-07-09T17:42:13Z

This pull request introduces 1 alert when merging d66725960b6f307a8c4a0d2bbb5a55dff69e1ae6 into e8b6c5c - view on LGTM.com

new alerts:

1 for Unused import

ysmanman · 2021-07-13T21:42:59Z

scripts/voqutil

+
+# ========================== Common VOQ util logic ==========================
+
+SYSTEM_PORT_TABLE_PREFIX = "SYSTEM_PORT_TABLE:"


Is the separator or table name defined in some python lib? If so, it would be great to import them, instead of hardcoding them.

@vganesan-nokia can you address @ysmanman's comment ?

Yes. I'll fix this if the table names are defined in some python lib.

Fixed for table names

arlakshm

as comments

arlakshm · 2021-07-19T20:49:32Z

scripts/voqutil

+
+# ========================== Common VOQ util logic ==========================
+
+SYSTEM_PORT_TABLE_PREFIX = "SYSTEM_PORT_TABLE:"


@vganesan-nokia can you address @ysmanman's comment ?

arlakshm · 2021-07-19T20:50:59Z

show/chassis_modules.py

+    if ipaddress is not None:
+        cmd += " -a {}".format(ipaddress)
+
+    if asicname is not None:


@vganesan-nokia this change will not work on the single asic linecards right ?
I think it will be better to the add the asicname check only on multi asic linecards ?

Please note that for system neighbors and system lags this is not namespace. It is the value of DEVICE_METADATA['localhost']['asic_name'] we filter on in keys . Therefore, this will/should work on single asic linecards also for the following reasons: (1) Even for the single asic line card, in voq chassis, it is mandatory to configure DEVICE_METADATA['localhost']['asic_name']. For single asic linecards, it may be always "asic0" or "Asic0". (2) Even if the command is executed from single asic linecard, we should be able to filter the entries (from chassis app db which has info for all asics from all linecards) for asic name of other line cards that have multiple asics.

arlakshm · 2021-07-19T20:51:27Z

tests/chassis_modules_test.py

+-------------------------------  --------  -----------  ------------------------------------------------------
+Linecard2|Asic1|PortChannel0002         1            8  Linecard2|Asic1|Ethernet16, Linecard2|Asic1|Ethernet17
+"""
+
 class TestChassisModules(object):


Do we need to add tests for single asic line cards ?

Yes. Please see my response to comment on chassis_modules.py:139-142

arlakshm · 2021-07-19T20:59:18Z

show/chassis_modules.py

+
+@chassis.command()
+@click.argument('systemlagname', required=False)
+@click.option('--asicname', '-x', 'asicname', default=None, type=str, show_default=False, help='Asic name')


why not use -n here ? similar to show chassis system_ports

For system neighbors and system lags the info are retrieved from supervisor card's chassis app db which is not in any particular namespace. The 'asicname' here is not the namespace. It is the 'asic_name' we look for in the keys of the entries.

Changed "asicname" option selector to "-n" as suggested

arlakshm · 2021-07-19T20:59:26Z

show/chassis_modules.py

+
+@chassis.command()
+@click.argument('ipaddress', required=False)
+@click.option('--asicname', '-x', 'asicname', default=None, type=str, show_default=False, help='Asic name')


why not use -n here ? similar to show chassis system_ports

Please see my response to comments on line# 146-149

Changed "asicname" option selector to "-n" as suggested

Implementation of the following show commands for VOQ chassis. - show chassis system-ports [<system port>] [-n <namespace>] [-d all] - show chassis system-neighbors - show chassis system-lags [<system lag>] The existing chassis commands under "show chassis_modules" and "config chassis_modules" have been renamed (moved) under "show chassis modules" and "config chassis modules" respectively Signed-off-by: vedganes <[email protected]>

Signed-off-by: vedganes <[email protected]>

Following changes done to address review comments - "-d" option is removed since this is not applicable - namespace option is made "required" for "show system-ports" command - "-x <asic name>" is used instead of namespace for "show system-neibhbors" and "show system-lags" since chassis db is namespace agnostic and also to take care of handling the case when the asic name many not match the namespace - "ipaddress" argument is introduced for "show system-neighbors" command to show info for specific neighbor. Signed-off-by: vedganes <[email protected]>

- LGTM warning fix. Removed unused import - Adjusted system port namespace option "not required" for single asic line cards. Signed-off-by: vedganes <[email protected]>

- Hard coded tables names are changed to use from swsscommon schema definition - Added option for linecard/host name filtering on system lag. Signed-off-by: vedganes <[email protected]>

- Changed asicname option from "-x" to "-n" - Changed hostname option to linecardname. Signed-off-by: vedganes <[email protected]>

arlakshm · 2021-08-18T20:25:54Z

/Azp run

azure-pipelines · 2021-08-18T20:26:05Z

Azure Pipelines successfully started running 1 pipeline(s).

Changes in this commit - Implementation of the following show commands for VOQ chassis. show chassis system-ports [<system port>] {-n <namespace>}, if multi-asic voq chassis show chassis system-ports [<system port>] , if single asic voq chassis show chassis system-neighbors [<nbr ip address>] [-n <asic name>] show chassis system-lags [<system lag>] [-n <asic name>] [-l <linecard or host name>] - The existing chassis commands under "show chassis_modules" and "config chassis_modules" have been renamed (moved) under "show chassis modules" and "config chassis modules" respectively Signed-off-by: vedganes <[email protected]>

vganesan-nokia force-pushed the voqcli-1 branch from 9932b10 to b1f222c Compare June 24, 2021 19:18

anshuv-mfst requested review from arlakshm and judyjoseph June 30, 2021 16:14

arlakshm reviewed Jul 7, 2021

View reviewed changes

ysmanman reviewed Jul 13, 2021

View reviewed changes

arlakshm reviewed Jul 19, 2021

View reviewed changes

arlakshm mentioned this pull request Jul 22, 2021

Changing show chassis-module cli command as per PR 805 sonic-net/sonic-mgmt#3762

Merged

3 tasks

vganesan-nokia force-pushed the voqcli-1 branch from 43e3c4b to 1524777 Compare July 28, 2021 02:49

judyjoseph added the Request for 202106 Branch label Aug 4, 2021

vedganes added 7 commits August 6, 2021 10:40

[voqcli] VOQ CLI show commands impl. LGTM warnings fix

70b429c

Signed-off-by: vedganes <[email protected]>

[voqcli] Show commands impl. Build failure fix - 1

5e8c7e3

Signed-off-by: vedganes <[email protected]>

[voqcli] LGTM warning fix 2

3d08fd6

- LGTM warning fix. Removed unused import - Adjusted system port namespace option "not required" for single asic line cards. Signed-off-by: vedganes <[email protected]>

[voq][cli] Review comments fix 2

480f89d

- Hard coded tables names are changed to use from swsscommon schema definition - Added option for linecard/host name filtering on system lag. Signed-off-by: vedganes <[email protected]>

[voqcli] Review comments fix 3

1741bf5

- Changed asicname option from "-x" to "-n" - Changed hostname option to linecardname. Signed-off-by: vedganes <[email protected]>

vganesan-nokia force-pushed the voqcli-1 branch from 1524777 to 1741bf5 Compare August 6, 2021 19:37

judyjoseph added the Chassis label Aug 10, 2021

arlakshm approved these changes Aug 11, 2021

View reviewed changes

arlakshm merged commit 7ebb2f7 into sonic-net:master Aug 25, 2021

judyjoseph added the Included in 202106 Branch label Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[voq] VOQ cli show commands implementation #1689

[voq] VOQ cli show commands implementation #1689

vganesan-nokia commented Jun 22, 2021 •

edited

Loading

lgtm-com bot commented Jun 22, 2021

anshuv-mfst commented Jun 30, 2021

arlakshm left a comment

arlakshm Jul 7, 2021

vganesan-nokia Jul 7, 2021

vganesan-nokia Jul 9, 2021

arlakshm Jul 7, 2021

vganesan-nokia Jul 9, 2021

arlakshm Jul 7, 2021

vganesan-nokia Jul 9, 2021

arlakshm Jul 7, 2021

vganesan-nokia Jul 9, 2021

lgtm-com bot commented Jul 9, 2021

ysmanman Jul 13, 2021

arlakshm Jul 19, 2021

vganesan-nokia Jul 27, 2021

vganesan-nokia Jul 28, 2021

arlakshm left a comment

arlakshm Jul 19, 2021

arlakshm Jul 19, 2021

vganesan-nokia Jul 27, 2021 •

edited

Loading

arlakshm Jul 19, 2021

vganesan-nokia Jul 27, 2021

arlakshm Jul 19, 2021

vganesan-nokia Jul 27, 2021

vganesan-nokia Aug 6, 2021 •

edited

Loading

arlakshm Jul 19, 2021

vganesan-nokia Jul 27, 2021

vganesan-nokia Aug 6, 2021

arlakshm commented Aug 18, 2021

azure-pipelines bot commented Aug 18, 2021


		# ========================== Common VOQ util logic ==========================

		SYSTEM_PORT_TABLE_PREFIX = "SYSTEM_PORT_TABLE:"

[voq] VOQ cli show commands implementation #1689

[voq] VOQ cli show commands implementation #1689

Conversation

vganesan-nokia commented Jun 22, 2021 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com bot commented Jun 22, 2021

anshuv-mfst commented Jun 30, 2021

arlakshm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vganesan-nokia Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vganesan-nokia Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm commented Aug 18, 2021

azure-pipelines bot commented Aug 18, 2021

vganesan-nokia commented Jun 22, 2021 •

edited

Loading

vganesan-nokia Jul 27, 2021 •

edited

Loading

vganesan-nokia Aug 6, 2021 •

edited

Loading